Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate unique id for device peripherals #857

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gephaistos
Copy link
Contributor

@gephaistos gephaistos commented Jul 23, 2024

Hi! I encoutered errors when compiling project with multiple devices used and lto enabled.
The problem is similar to this issue with generated variable DEVICE_PERIPHERALS.
This PR solves the problem by generating unique name based on the device name removing #[no_mangle] attribute.

@gephaistos gephaistos requested a review from a team as a code owner July 23, 2024 12:16
@jannic
Copy link
Member

jannic commented Jul 26, 2024

Not a complaint about your solution - AFAIKT it should work - but I wonder:

The comment above DEVICE_PERIPHERALS states:

        // NOTE `no_mangle` is used here to prevent linking different minor versions of the device
        // crate as that would let you `take` the device peripherals more than once (one per minor
        // version)

If you only get errors with LTO enabled, is the purpose stated in that comment reached at all? If it doesn't work anyway, we could as well remove the no_mangle and avoid the complexity of generating a unique name.

@gephaistos gephaistos changed the title Generate unique id for device peripherals Remove #[no_mangle] attribute for device peripherals Jul 26, 2024
@Emilgardis
Copy link
Member

If you only get errors with LTO enabled, is the purpose stated in that comment reached at all? If it doesn't work anyway, we could as well remove the no_mangle and avoid the complexity of generating a unique name.

That's certainly a bug though? The idea is that, if it works, you should not be able to easily claim two mutable references using two different versions of the library.

I think the no_mangle should stay and a name corresponding to the device name to be used, its still useful, and also useful even with non-mut static (in the future 2024 edition).

@Shatur
Copy link
Contributor

Shatur commented Jul 26, 2024

I'm not sure if this is the intended way to use no_mangle.

@Emilgardis
Copy link
Member

To me it seems like a fair way to use it, but maybe we could (ab)use package.links instead.

@Emilgardis
Copy link
Member

no_mangle makes a symbol visible (unconditionally) under that exact name. Why ThinLTO allows this is very strange

@gephaistos gephaistos changed the title Remove #[no_mangle] attribute for device peripherals Generate unique id for device peripherals Jul 29, 2024
@jannic
Copy link
Member

jannic commented Jul 29, 2024

no_mangle makes a symbol visible (unconditionally) under that exact name. Why ThinLTO allows this is very strange

rust-lang/rust#28179 may be relevant here.

Money quote: "I'd argue that having multiply-defined #[no_mangle] symbols is, in essence, undefined behavior (in the sense that nobody defined it)"

@adamgreig
Copy link
Member

We discussed this a bit in the meeting today.

  • Renaming it from DEVICE_PERIPHERALS means that new versions of a PAC could link with old versions, which is unsound
  • Same problem with swapping just to links in Cargo.toml
  • I think as long as we provide the safely-acquired owned singletons, we have to keep this no_mangle DEVICE_PERIPHERALS, even though it kinda sucks. As far as I can tell it does usually prevent linking together.
  • We could maybe add a runtime feature flag to disable acquiring owned singletons and thus also disable the DEVICE_PERIPHERALS static, which would allow two PACs to be linked together (but both would only provide unsafe APIs for obtaining the peripherals)

@jannic
Copy link
Member

jannic commented Aug 6, 2024

* I think as long as we provide the safely-acquired owned singletons, we have to keep this `no_mangle` `DEVICE_PERIPHERALS`, even though it kinda sucks. As far as I can tell it does _usually_ prevent linking together.

Just as a data point: I did some quick tests using rp2040-pac. Compiling a program calling Peripherals::take().unwrap() from both rp2040-pac 0.5.0 and 0.6.0 from main succeeds or fails depending on the lto settings:

  • false (and codegen-units=2): fails with linker error rust-lld: error: duplicate symbol: DEVICE_PERIPHERALS
  • off: fails with linker error rust-lld: error: duplicate symbol: DEVICE_PERIPHERALS
  • thin: succeeds without warning
  • fat: compiler warning warning: Linking globals named 'DEVICE_PERIPHERALS': symbol multiply defined! followed by compiler error error: failed to load bitcode of module "rp2040_pac-bff007a1c8be64d8.rp2040_pac.8071c7cd85106f05-cgu.0.rcgu.o"

This is the same for both release and debug builds.

@adamgreig
Copy link
Member

Thanks, it's useful to know it's only a problem on thin LTO. Perhaps another good reason to plan to move away from owned singletons given out like this.

@jannic
Copy link
Member

jannic commented Aug 13, 2024

Related: rust-embedded/wg#467

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants